Fix settings panel reset during state update #1051

System233 · 2025-02-18T14:38:47Z

Description

Add Save/Done buttons to allow users to cancel setting changes.
Decouple ApiOptions from ExtensionStateContext, enabling the settings panel to edit on a snapshot of the apiConfiguration, fixing the issue of settings panel reset during API response completion, profile renaming, and any state updates.
Improve the event handler for handleInputChange in ApiOptions.
Only save settings when clicking the Save button to avoid accidental changes.
Switching profiles will no longer save settings

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Checklist:

My code follows the patterns of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation

Additional context

Related Issues

Reviewers

Important

Fixes settings panel reset issue by decoupling ApiOptions from ExtensionStateContext and adding Save/Done buttons for explicit configuration changes.

Behavior:
- Adds Save/Done buttons in SettingsView.tsx to allow users to save or discard changes.
- Decouples ApiOptions from ExtensionStateContext, allowing editing on a snapshot of apiConfiguration.
- Changes in ApiOptions.tsx ensure settings are only saved when Save is clicked.
- Switching profiles no longer saves settings automatically.
Functions:
- Introduces onApiConfigurationUpdate in SettingsView.tsx to handle API configuration updates.
- Refactors handleInputChange in ApiOptions.tsx to use onApiConfigurationUpdate.
Tests:
- Updates tests in ApiOptions.test.tsx and SettingsView.test.tsx to reflect new Save/Done behavior.
- Ensures tests check for the presence of Save button and correct message posting to VSCode.

^{This description was created by}^{for a62e6a78fdd15b483f54befb7bcb6c20964b5389. It will automatically update as commits are pushed.}

changeset-bot · 2025-02-18T14:38:50Z

⚠️ No Changeset found

Latest commit: eeb1e45

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

webview-ui/src/components/settings/ApiOptions.tsx

webview-ui/src/components/settings/SettingsView.tsx

mrubens · 2025-02-18T16:08:53Z

Maybe it should warn you if you switch profiles or leave the settings tab with unsaved changes?

System233 · 2025-02-18T16:12:55Z

Maybe it should warn you if you switch profiles or leave the settings tab with unsaved changes?

I am considering it, but webview-ui cannot call vscode.window.showInformationMessage, is there any good way?

this prompt box not pretty

cte · 2025-02-18T17:29:45Z

I am considering it, but webview-ui cannot call vscode.window.showInformationMessage, is there any good way?

We might want to use https://ui.shadcn.com/docs/components/alert-dialog

I can make it look at little bit nicer / less heavy-handed.

System233 · 2025-02-18T17:33:00Z

I am considering it, but webview-ui cannot call , is there any good way?vscode.window.showInformationMessage

We might want to use https://ui.shadcn.com/docs/components/alert-dialog

I can make it look at little bit nicer / less heavy-handed.

simply adjusted the transparency and it looks good now.

cte · 2025-02-18T17:40:04Z

simply adjusted the transparency and it looks good now.

Looks nice!

System233 · 2025-02-18T18:21:04Z

Due to the optimization of the save logic, saveApiConfiguration is no longer needed for now.
Unless explicitly click the Save button, the config will not be saved at any time.

Final result:

cte · 2025-02-19T07:49:09Z

webview-ui/src/components/ui/comfirm-dialog.tsx

Not a blocker, but we might want to use the official shadcn version of this component:

npx shadcn@latest add alert-dialog

Many radix-ui components are already in use, it also provides an Alert Dialog, and we only use the Dropdown from the vscrui , I’m considering switching these to radix-ui.

@System233 i think we can use shadcn too, shadcn built on radix-ui with tailwind theme support, and @cte already config theme for tailwind and shadcn component

@System233 i think we can use shadcn too, shadcn built on radix-ui with tailwind theme support, and @cte already config theme for tailwind and shadcn component

The shadcn works way is better than I expected, thanks for reminder.

cte · 2025-02-19T07:50:31Z

webview-ui/src/components/settings/__tests__/SettingsView.test.tsx

I think this test is failing:

● SettingsView - Allowed Commands › saves allowed commands when clicking Save expect(jest.fn()).toHaveBeenCalled() Expected number of calls: >= 1 Received number of calls: 0 328 | }), 329 | ) > 330 | expect(onDone).toHaveBeenCalled() | ^ 331 | }) 332 | }) 333 | at Object.<anonymous> (src/components/settings/__tests__/SettingsView.test.tsx:330:18)

This reminds me that there are still some fields that have not been snapshotted.

cte · 2025-02-19T08:01:11Z

webview-ui/src/components/settings/ApiOptions.tsx

Nit-pick: I'd probably name these something like:

inputEventTransform

dropdownEventTransform

noTransform

I think this makes it clear that they are transform function rather than actual event handlers.

cte

Nice work! I tested this locally and it's a big improvement IMO. We just need to fix the one failing test.

I left some non-blocking feedback; feel free to ignore.

System233 · 2025-02-19T22:49:14Z

Adjusted the following UI to make it compatible with all themes

Unit-test

I'm not sure what this is

I added mock data to api_conversation_history.json and then I got this

<--- Last few GCs --->

[36880:000002621C559000]    47972 ms: Scavenge 4056.6 (4125.2) -> 4045.0 (4128.0) MB, pooled: 0 MB, 5.77 / 0.00 ms  (average mu = 0.163, current mu = 0.194) allocation failure;
[36880:000002621C559000]    48861 ms: Mark-Compact 4059.8 (4128.5) -> 4046.9 (4131.7) MB, pooled: 0 MB, 876.10 / 0.00 ms  (average mu = 0.194, current mu = 0.224) allocation failure; scavenge might not succeed


<--- JS stacktrace --->

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
----- Native stack trace -----

 1: 00007FF6058520AD node::SetCppgcReference+17245
 2: 00007FF6057BA4D8 v8::base::CPU::num_virtual_address_bits+92376
 3: 00007FF60633A1B1 v8::Isolate::ReportExternalAllocationLimitReached+65
 4: 00007FF606327096 v8::Function::Experimental_IsNopFunction+2790
 5: 00007FF606176920 v8::internal::StrongRootAllocatorBase::StrongRootAllocatorBase+31392
 6: 00007FF606170644 v8::internal::StrongRootAllocatorBase::StrongRootAllocatorBase+6084
 7: 00007FF60616BCF5 v8::CpuProfileNode::GetScriptResourceNameStr+188069
 8: 00007FF605AF13BD BIO_ssl_shutdown+189

System233 · 2025-02-19T22:56:25Z

mrubens · 2025-02-19T23:20:40Z

Tests are flaky/broken in main unfortunately

mrubens

👏

mrubens · 2025-02-20T02:09:39Z

webview-ui/src/components/settings/TemperatureControl.tsx

Actually I'm not sure about changing this to a slider - not all of the providers have the same 0-2 range. And separately, I'm noticing an error where the value gets reset to 1.

Screen.Recording.2025-02-19.at.9.09.17.PM.mov

Can we just leave as a text input for now?

sure. 1 is the default value, other values can also be the default values

range 0-2 comes from the parent component, and default value is set when isCustomTemperature is toggled, these behaviors are the same as before the change, is it really bad after changing to a slider?

bandicam.2025-02-20.10-50-09-663.mp4

Ah, I guess I was confused since 0 used to be the default value. I don't feel super strongly against the slider and it's certainly nicer in some ways, I just think it implies a valid range of 0-2 that's not the case for all models.

Ah, I guess I was confused since 0 used to be the default value. I don't feel super strongly against the slider and it's certainly nicer in some ways, I just think it implies a valid range of 0-2 that's not the case for all models.

I'm not sure either, I didn't change maxValue, and I've restored the default value to 0

If you like the slider better let’s go for it then

mrubens · 2025-02-20T02:13:12Z

Actually I'm seeing some weirdness on rename now as well

Screen.Recording.2025-02-19.at.9.12.18.PM.mov

System233 · 2025-02-20T02:18:24Z

Actually I'm seeing some weirdness on rename now as well

Screen.Recording.2025-02-19.at.9.12.18.PM.mov

It was fine before, I'll fix it

mrubens · 2025-02-20T03:20:35Z

Rename seems to be working better, thanks! Looks like just one unit test, assuming that failure is legit.

System233 · 2025-02-20T03:24:53Z

Rename seems to be working better, thanks! Looks like just one unit test, assuming that failure is legit.

The slider is debounced, so there need to delay and assert it.
My mistake, fix it now.

mrubens · 2025-02-20T03:54:07Z

🚀

System233 · 2025-02-22T11:00:15Z

@mrubens I found that there were still references to the ExtensionStateContext in the ModelPickers and those settings were not being snapshotted, working on fixing that.

At the same time, I found a potential issue: due to the wrong type constraint, openAiCustomModelInfo was actually stored as openAiModelInfo

System233 requested review from ColemanRoo, cte, mrubens and stea9499 as code owners February 18, 2025 14:38

ellipsis-dev bot reviewed Feb 18, 2025

View reviewed changes

webview-ui/src/components/settings/ApiOptions.tsx Outdated Show resolved Hide resolved

System233 marked this pull request as draft February 18, 2025 14:45

System233 force-pushed the patch-settings-panel-rename branch from 0a3af39 to a3b29a6 Compare February 18, 2025 14:59

System233 marked this pull request as ready for review February 18, 2025 15:03

System233 mentioned this pull request Feb 18, 2025

Fix input box revert issue and configuration loss during profile switch #955 #979

Merged

8 tasks

System233 marked this pull request as draft February 18, 2025 15:34

System233 force-pushed the patch-settings-panel-rename branch from a3b29a6 to a62e6a7 Compare February 18, 2025 15:48

System233 marked this pull request as ready for review February 18, 2025 15:53

ellipsis-dev bot reviewed Feb 18, 2025

View reviewed changes

webview-ui/src/components/settings/ApiOptions.tsx Outdated Show resolved Hide resolved

webview-ui/src/components/settings/SettingsView.tsx Outdated Show resolved Hide resolved

System233 force-pushed the patch-settings-panel-rename branch from a62e6a7 to 06382ca Compare February 18, 2025 16:06

System233 force-pushed the patch-settings-panel-rename branch from 581eb48 to aeda74b Compare February 18, 2025 21:49

cte reviewed Feb 19, 2025

View reviewed changes

cte approved these changes Feb 19, 2025

View reviewed changes

System233 force-pushed the patch-settings-panel-rename branch from aeda74b to 6cf19b6 Compare February 19, 2025 22:10

System233 force-pushed the patch-settings-panel-rename branch from 3ec352a to 6cf19b6 Compare February 19, 2025 22:56

mrubens approved these changes Feb 20, 2025

View reviewed changes

mrubens reviewed Feb 20, 2025

View reviewed changes

System233 added 5 commits February 20, 2025 10:28

Fix settings panel reset during state update

2036e84

Add confirmation dialog for discarding changes.

1fcdd33

Fix the sticky background color of the welcome view

e5f5cbc

Add icon feedback for apiErrorMessage/modelIdErrorMessage

2d34cda

fix: getunbound.ai may return null

b732005

System233 force-pushed the patch-settings-panel-rename branch from 6cf19b6 to 13c902a Compare February 20, 2025 02:54

Improve UI experience

eeb1e45

System233 force-pushed the patch-settings-panel-rename branch from 13c902a to eeb1e45 Compare February 20, 2025 03:28

mrubens merged commit e6f3bb7 into RooCodeInc:main Feb 20, 2025
6 checks passed

joemanley201 mentioned this pull request Feb 20, 2025

Handle errors more gracefully when reading custom instructions from files #1093

Merged

8 tasks

Fix settings panel reset during state update #1051

Fix settings panel reset during state update #1051

Uh oh!

Conversation

System233 commented Feb 18, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Additional context

Related Issues

Reviewers

Uh oh!

changeset-bot bot commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrubens commented Feb 18, 2025

Uh oh!

System233 commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cte commented Feb 18, 2025

Uh oh!

System233 commented Feb 18, 2025

Uh oh!

cte commented Feb 18, 2025

Uh oh!

System233 commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

System233 Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

System233 Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cte left a comment

Choose a reason for hiding this comment

Uh oh!

System233 commented Feb 19, 2025

Unit-test

Uh oh!

System233 commented Feb 19, 2025

Uh oh!

mrubens commented Feb 19, 2025

Uh oh!

mrubens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

System233 commented Feb 18, 2025 •

edited by ellipsis-dev bot

Loading

changeset-bot bot commented Feb 18, 2025 •

edited

Loading

System233 commented Feb 18, 2025 •

edited

Loading

System233 commented Feb 18, 2025 •

edited

Loading

System233 Feb 19, 2025 •

edited

Loading

System233 Feb 19, 2025 •

edited

Loading

mrubens commented Feb 20, 2025 •

edited

Loading

System233 commented Feb 20, 2025 •

edited

Loading

System233 commented Feb 22, 2025 •

edited

Loading